AtomicCounter refactor#21
Merged
Merged
Conversation
msizov
force-pushed
the
feat/atomic-counter-manager
branch
from
July 1, 2026 11:20
ef575f0 to
364991e
Compare
tla/ChotkiSync.tla models the replication protocol end to end: the Syncer feed/drain state machines, pebble-snapshot diff sync with sync-point batches applied on 'V', broadcast relaying over a replica tree, and network read batching (record coalescing). TLC configs check the fixed protocol (NoGaps, exact/complete block VVs, one diff sync per src, quiescent convergence - all pass, see tla/README.md) and reproduce three real protocol bugs as counterexamples: 1. Drain() did not rebroadcast a batch whose last record was 'B' (bye), nor the applied prefix of a batch failing mid-way. Records applied at the middle replica never reached downstream ones, and the next re-handshake merged the origin's version vector downstream without the data, making the loss permanent and silent. Drain now relays exactly the locally-applied prefix minus the session-scoped trailing 'B' (Chotki.DrainApplied + Syncer.relayApplied). 2. The "allow only 1 diff sync per src" cleanup in drain() 'H' compared sync-point keys against cho.src, which never matches since a replica never drains its own handshake; stale sync points were never displaced. It now compares against the incoming handshake's source. 3. (found by TLC) A sync point outlived the session whose relayed 'H' created it: when a downstream link reconnected mid-relay, the relayed 'D' records died with the old connection while the relayed 'V' arrived over the new one and applied the staged handshake VV without the data - permanent silent data loss on any network blip during a relayed diff sync. Sync points are now bound to the session that created them (Syncer.SessionId carried in the drain context) and aborted when it closes (Chotki.AbortSyncsVia). Also fixed along the way (details in tla/README.md): - draining a peer's bye no longer cuts our own diff short of its 'V' (the peer would silently miss the promised data; TestChotki_Sync3 flaked on this) - Syncer.Close raced concurrent Feed over the pebble snapshot iterators (mergingIter panic under -race); guarded by snapLock - Feed's SendNone wait could block forever when a late Drain moved the drain state backwards after the one-shot unblock timer had fired - WaitDrainState leaked two goroutines per call; processPings skipped the record following a removed ping; Feed's SendPing case mutated pingTimer without the lock - ApplyD/ApplyV looped forever on truncated inner TLV records; ApplyV overwrote earlier errors; ApplyH merged a nil VV from a malformed relayed handshake; drain() 'H' registered the sync point even when ApplyH failed - test fixes: examples left a DB open racing its cleanup; index tests waited only 5s for the background reindex worker
Each test fails against the pre-fix code:
- TestDrainRelaysAppliedRecordsWhenBatchEndsWithBye: a drained batch
coalesced with the peer's 'B' (bye) must still be rebroadcast to the
other sessions, minus the session-scoped trailing bye.
- TestHandshakeDisplacesStaleSyncPointOfSameSrc: a new handshake
displaces the origin's older, unfinished sync point ("allow only 1
diff sync per src").
- TestSyncerCloseAbortsItsSyncPoints: sync points die with the session
that created them; a late 'V' gets ErrSyncUnknown instead of applying
the staged handshake VV without the data.
- TestFeedFinishesDiffAfterPeerBye: a peer's bye must not cut our own
diff short of its 'V' packet.
- TestApplyDVMalformedInputFailsFast: truncated inner TLV records fail
fast instead of looping forever.
- TestProcessPingsFiltersAllPingsAndKeepsTheRest: ping filtering no
longer skips the record following a removed ping.
- TestWaitDrainState*: no goroutine leaks for non-cancellable contexts
or abandoned waits.
CommitPacket stamps new local ids from the cached cho.last under commitMutex, while replication sessions can also advance cho.last when they drain records stamped with our own src (our own history synced back after a restore from an older snapshot) - holding only cho.lock.RLock. The two paths share no lock, so a commit could observe a stale cho.last and reuse an already issued seq: two different mutations under one rdx.ID. rdx.ID is two uint64s, so a torn read was possible on top of the lost update. MCBuggyLast.cfg is the TLA+ witness of the reuse; MCFixedLast.cfg models this fix. Give the allocator cache its own mutex (lastLock) around the commit allocation (nextLast), the own-source advance in drain, Last() and the Close() reset. TestCommitAllocatorSyncedWithOwnSourceDrain races the two paths and fails (under -race) against the pre-fix code.
msizov
force-pushed
the
feat/atomic-counter-manager
branch
from
July 8, 2026 13:13
364991e to
67f4900
Compare
a10zn8
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hot path for counter increments/retrival is lock-free, synchronization/flushes moved to separate manager routine
New CommitBatch method used for sync/flushes by manager routine